Skip to content

attrs: remove fields type check#15983

Merged
hauntsaninja merged 1 commit into
python:masterfrom
ikonst:attrs-remove-fields-type-check
Aug 30, 2023
Merged

attrs: remove fields type check#15983
hauntsaninja merged 1 commit into
python:masterfrom
ikonst:attrs-remove-fields-type-check

Conversation

@ikonst

@ikonst ikonst commented Aug 29, 2023

Copy link
Copy Markdown
Contributor

Since python-attrs/attrs#890 (≥ 22.1.0) attrs.fields is typed to accept a protocol.
Since python-attrs/attrs#997 (≥ 22.2.0) attrs.has is a type-guard.

Support both by removing the explicit error reporting and letting it fall through to the type stub.

Fixes #15980.

@ikonst ikonst force-pushed the attrs-remove-fields-type-check branch from 8d9cfc1 to 0bca9e9 Compare August 29, 2023 04:50
@github-actions

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

prefect (https://github.com/PrefectHQ/prefect): typechecking got 1.10x slower (110.1s -> 120.5s)
(Performance measurements are based on a single noisy sample)

@ikonst

ikonst commented Aug 29, 2023

Copy link
Copy Markdown
Contributor Author

@sobolevn this is along the same lines as #15962

btw, the reason why I'm not culling the same code (wrote it at the same time...) for attrs.replace is that its signature is still attrs.replace(obj: T, ...) where T is not bound, i.e. the typeshed offers no validation of the first arg being an attrs instance.

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

@hauntsaninja hauntsaninja merged commit 2298829 into python:master Aug 30, 2023
@ikonst ikonst deleted the attrs-remove-fields-type-check branch August 30, 2023 01:10
@Tinche

Tinche commented Nov 10, 2023

Copy link
Copy Markdown
Contributor

@ikonst haha thanks for this, I had it on my mental checklist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for attr.has and attr.AttrsInstance

4 participants